-
Notifications
You must be signed in to change notification settings - Fork 65
improve returned data if running a simulation (using the model executable) #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| cwd=cmd_run_data.cmd_cwd_local, | ||
| timeout=cmd_run_data.cmd_timeout, | ||
| check=True, | ||
| check=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting an exception on non-zero exit code was good. Exceptions for handling control flow in Python are totally fine, and obvious to the user, and hard to miss, I would keep that. (I did not realize that that's what's happening because I did not dig deep enough in the call hierarchy)
The additional logging is good, so I'd keep that too. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception on non-zero exit code has to be handled with care. There are possibilities that a result file is created (and usefull) even if the exit code is != 0. An exception here would prevent any processing of such data.
If I remeber correctly, an example is if the model is terminated via Modelica code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remeber correctly, an example is if the model is terminated via Modelica code.
You mean via terminate()? If yes a nonzero exit code would be strange, because this "successfully terminates the analysis which was carried out".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, this change is fine. It’s also consistent with how we handle this in OMEdit: we show the user what happened when running the executable, and if there’s any useful output, we use it for post-processing. If an exception is raised, that post-processing will not happen anyway.
Regarding check=True, it does the following:
If check is True and the exit code is non-zero, a CalledProcessError is raised. The CalledProcessError object contains the return code in the returncode attribute, and the output and stderr attributes if those streams were captured.
The new code already covers this behavior: it explicitly checks the returncode and logs it.
…table * use check=False => no CalledProcessError exception; possibility to handle the error code * error code needs to be checked (compare == 0) by the caller! * improve log messages; print returncode if it is != 0 with stdout
* ensure a message if logged if returncode != 0
* self.session().run_model_executable() will raise OMCSessionException!
see #108
[OMCSession] improve log messages for model simulation using OM executable
[ModelicaSystem] improve handling of model simulation